Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow repositories without an 'origin' remote #353

Merged
merged 4 commits into from
Mar 17, 2020
Merged

Allow repositories without an 'origin' remote #353

merged 4 commits into from
Mar 17, 2020

Conversation

derrickstolee
Copy link
Contributor

Resolves #350

While playing around with an empty repository, I found that scalar diagnose was failing because it was adding src to the working directory. Turns out there is a missed path that happens depending on the verb options, and now it is fixed.

While doing that, improve the message for InvalidRepoException.

@derrickstolee
Enlistment: be robust to repos with no 'origin'
9c8bd99 
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee
Enlistment: allow repos without 'origin'
0827468 
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
jrbriggs
jrbriggs approved these changes Mar 17, 2020
View reviewed changes
Copy link
Member

@jrbriggs jrbriggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with suggestion: While we're here, let's clean up the documentation about background fetch, which still makes mention of origin, even though it actually iterates your remotes without regard to what they're named.

@derrickstolee
ScalarEnlistment: properly store working directory when creating with…
e1b2a97 
…out URL

Remove the other constructor that allows this by default to avoid this
problem in the future.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee
Docs: update fetch step about multiple remotes:
d09d345 
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee
Copy link
Contributor Author

Approved with suggestion: While we're here, let's clean up the documentation about background fetch, which still makes mention of origin, even though it actually iterates your remotes without regard to what they're named.

Good find. The reference to origin is correct when talking about the GVFS protocol, so I added a sentence about the non-GVFS protocol case and how it will fetch from each remote.

I also figured it would be good to remove that problematic constructor from ScalarEnlistment so we don't hit the src folder thing again. Now the CloneVerb is the only place that knows to add the src directory when creating a ScalarEnlistment.

kewillford
kewillford reviewed Mar 17, 2020
View reviewed changes
Scalar.Common/Enlistment.cs
throw new InvalidRepoException(this.WorkingDirectoryRoot, $"Failed to load remotes with error: {error}");
}

if (remotes.Length > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be remotes.Length == 1? If there are more that one are we sure we are doing the right thing to just take the first remote? Should there be a config setting for that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it safer to have a RepoUrl setting rather than have none.

I think it is important that we will not support multiple remotes when using the GVFS protocol, and I think that's the only time we use RepoUrl (other than logging).

@derrickstolee derrickstolee merged commit c559f5e into microsoft:master Mar 17, 2020
@derrickstolee derrickstolee mentioned this pull request Mar 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kewillford kewillford kewillford left review comments

@jrbriggs jrbriggs jrbriggs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

scalar register fails if there is no 'origin' origin
3 participants
@derrickstolee @jrbriggs @kewillford